Answer the review on the include work - #8
Conversation
Refuse a catalog entry that already carries its rubric, share the ownership predicate the two publish loops had each re-derived, fetch the project root once, and correct the schema paths that told authors a spliced path is rebased.
go fix -diff flags the slice walk in containsRefDirective, and lint-ext-azure-ai-evaluations.yml runs that gate against this module.
Asking whether the document used a directive anywhere let a ref on an unrelated dataset rewrite a hand-written dimensions key into a rubric, so the same evaluator was refused or accepted according to a neighbour.
There was a problem hiding this comment.
Pull request overview
This pull request improves Azure AI evaluations $ref handling, catalog safeguards, path resolution, schema documentation, and regression coverage.
Changes:
- Centralizes rubric ownership detection and catalog editability checks.
- Preserves include provenance and reuses project-root resolution.
- Updates schemas, documentation, messages, and tests.
Review findings:
- Critical (3 votes):
eval_config_store.goincorrectly treats all evaluators from a referenced configuration as evaluator-level includes, potentially publishing invalid entries. - Moderate (2 votes):
catalog.godrops evaluator version pins during editability checks. - Moderate (2 votes): The schema does not apply version-conflict validation to evaluator-specific
$refshapes.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Summary |
|---|---|
cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json |
Updates path documentation and version constraints; retains an unresolved $ref validation gap. |
cli/azd/extensions/azure.ai.evaluations/README.md |
Documents rubric include usage and editing behavior. |
cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go |
Reuses project-root resolution for deployment paths. |
cli/azd/extensions/azure.ai.evaluations/internal/project/one_ownership_test.go |
Enforces centralized ownership detection. |
cli/azd/extensions/azure.ai.evaluations/internal/project/neighbour_ref_test.go |
Tests entry-local include behavior. |
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go |
Adds shared rubric ownership logic. |
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go |
Tracks evaluator include entries; contains an unresolved provenance issue. |
cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go |
Adds catalog editability messages. |
cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go |
Uses shared ownership detection. |
cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go |
Guards catalog updates; version-pin handling remains unresolved. |
cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go |
Tests catalog include and inline-rubric guards. |
Suppressed comments (1)
cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go:163
- This guard is reached only after
runGenerationshas awaitedgenerateRubric; that function has already submitted the billed job and written the artifact beforeaddEvaluatorToCatalogreaches this call. An inline-rubric or include case therefore still incurs the generation cost and leaves an unrecorded file before returning this new error. Move the editability check into the pre-submission planning path, keeping this catalog check only as a backstop.
if err := checkCatalogEntryIsEditable(evalDir, cfg, kind, ref.Name); err != nil {
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| if decl, ok := cfg.EvaluatorDeclaration(name); ok { | ||
| return decl.Ref, true | ||
| return catalogEntryShape{ref: decl.Ref, inlineRubric: decl.Definition != nil}, true |
| if visible && !spliced[i] { | ||
| continue |
| "if": { "required": ["version"] }, | ||
| "then": { "properties": { "source": false, "definition": false } } |
A version pin says the rubric is already registered, so recording a generated file leaves the entry claiming both, which the next read rejects.
|
Three comments this round. One fixed, one deferred to a work item with reasoning, one the schema cannot express. 1. Correct, and the same class as the inline-rubric bug in this PR: a version pin says the rubric is already registered, so writing Note the asymmetry with datasets is deliberate and now covered by a test: a dataset may hold 2. Correct, and it is the limitation I called out when fixing the document-wide gate. When the service entry is itself a Not patched here, because every available fix is worse than the bug:
The real fix is to stop round-tripping this configuration through typed structs at all. Core already ships the mechanism — That is now a tracked work item and is being implemented next. 3. Version rules bypassed by the The mechanism is exactly as described: a The suggested rule would over-restrict, though. Whether an overlay Between a schema that misses an invalid shape and one that rejects a valid one, the first is the lesser harm, and the runtime rejects it with a message naming the entry index and the conflict. Leaving the runtime as the authority here. |
06a6251
into
feat/azure-ai-evaluations-consolidated
Answers the review on #9668, the slice opened because Copilot's re-review of #7 failed with "Copilot encountered an error and was unable to review this pull request" and #7 merged with that commit unreviewed.
Seven comments came back. Two were artifacts of how the slice was built and are not real. Three are fixed here. Two are pre-existing and out of scope, with evidence below. Three further bugs came from a parallel review of the same delta, and the highest-severity one was not in the review at all.
Fixed
An entry already carrying its rubric is refused by the catalog commands.
addEvaluatorToCatalogmatched onnameand wrotesource:into the entry whileDefinitionstayed populated. No\involved. The next read rejects the whole configuration for declaring the rubric twice, and it does so after the generation job has been billed and the file written. The guard now covers this shape alongside the two include shapes.The ownership predicate is shared.
source == "" && definition == nilexisted in two independent copies:CustomEvaluatorsforazd up, and an inline re-implementation ineval create. Only the first had a test. Reverting the second tosource == ""left the suite green andeval createsilently inert, which is the bug that shipped once already. Both now callEvaluatorDecl.CarriesItsRubric, and a structural test fails the build if a third copy appears.The project root is fetched once. It was fetched again for artifact paths after being used to resolve
\, so a second failure would have passed the include guard and then resolved artifacts against the extension process's directory.Schema paths no longer claim to be rebased.
file:andsource:said "relative to this file", which is wrong for an entry reached through a\-- the hover instructed authors to write a path deployment cannot find. Both now say what actually happens and steer todefinition:. The schema also expresses the twoversionconflicts the code already rejected.The modernization gate.
go fix -diff ./...flagged the slice walk incontainsRefDirective, andlint-ext-azure-ai-evaluations.ymlruns that gate against this module, so this would have failed CI on Azure#9500.Not fixed, with reasons
Mojibake in
README.mdandmessages.gois not real. The files on disk are valid UTF-8 with no replacement characters, anddoneMarkis the expected check mark. The slice was built by pipinggit showthrough PowerShell, which re-encoded the non-ASCII. My error, not the code's. The slice is now built withgit checkoutso the blobs are byte-identical.errors.Asandsort.Stringsare pre-existing and not flagged by the gate. Both predate this work and appear throughout the extension and incli/azd/pkg(40 and 5 occurrences).go fix -diffrewrites neither. Worth a separate sweep, not this PR.Tests
Dataset-kind guard coverage in both include shapes, the inline-rubric refusal, and the editable-in-place case that must not be caught. Each new guard was mutation-tested: the bug reintroduced, the test confirmed failing, the fix restored.